-
-
Notifications
You must be signed in to change notification settings - Fork 195
London|May-2025|KhilolaRustamova|Module-Structuring-&-Testing-Data|sprint 3 #636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
London|May-2025|KhilolaRustamova|Module-Structuring-&-Testing-Data|sprint 3 #636
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is generally looking good, I left a few things for you to think about.
|
||
if (num >= 2 && num <= 9) return num; | ||
|
||
throw new Error("Invalid card rank"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this exception is thrown, it may not be clear to the caller what the invalid rank was - can you think how to make this more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you have any thoughts here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new Error(Invalid card rank: "${rank}"from card "${card}"
); would be more clear to specify what card was wrong.
if (["K", "Q", "J", "10"].includes(rank)) return 10; | ||
|
||
const num = parseInt(rank); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, but: either of these lines of code could handle the value 10 - why did you decide to handle it in the K/Q/J case rather than the number case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By grouping "10" with "K", "Q", and "J", you make the intention clear:
"These are all cards that are worth 10 points, no matter what."
It also avoids need of extra range check (num === 10) in the number parsing block, which could make the logic more complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your thoughts on the grouping, but in terms of the code, I think a different grouping may make sense here.
"These are all cards whose value is exactly what's written" vs "These are cards we need to treat specially" feels like a more important difference to me than "These are cards worth 10 points".
And in terms of logic complexity, you already have an upper bound in your range check. So I think:
if (["K", "Q", "J"].includes(rank)) return 10;
// ...
if (num >= 2 && num <= 10) return num;
is actually simpler than
if (["K", "Q", "J", "10"].includes(rank)) return 10;
// ...
if (num >= 2 && num <= 9) return num;
Including "10" in the array involves writing more code. Either way you need an upper bound on the num
case, so the difference is really just whether you have an extra element in the array.
@@ -22,3 +22,6 @@ test("should count multiple occurrences of a character", () => { | |||
// And a character char that does not exist within the case-sensitive str, | |||
// When the function is called with these inputs, | |||
// Then it should return 0, indicating that no occurrences of the char were found in the case-sensitive str. | |||
|
|||
console.log(countChar("hello", "l")); // Should print 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are correct expectations, but they're not tests written in Jest - can you try to write actual tests for these cases?
Sprint-3/3-mandatory-practice/implement/get-ordinal-number.test.js
Outdated
Show resolved
Hide resolved
@@ -13,20 +13,36 @@ test("should repeat the string count times", () => { | |||
const str = "hello"; | |||
const count = 3; | |||
const repeatedStr = repeat(str, count); | |||
expect(repeatedStr).toEqual("hellohellohello"); | |||
expect(repeat(str, count)).toEqual("hellohellohello"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you change this line? What was broken about what it did before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(repeat(str, count)).toEqual("hellohellohello");
Calls the repeat() function directly inside the test.
It’s like saying: “Call the function now with str and count, and I expect it to return "hellohellohello".”
before it was saying "I expect the value stored in repeatedStr to equal "hellohellohello"
It might not be broken but i find it unnecessary step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is makes sense but you probably want to delete the repeatedStr
variable if you're going to do this.
Right now is slightly the worst of both situations - you have the extra variable but you're not using it which is confusing. I agree that calling repeat
directly in the assertion is fine and clear!
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.